First attempt at supporting https connections through proxy #186
Conversation
Connorhd
force-pushed
the
https-through-proxy-support
branch
from
ecf96cf
to
0bc0885
Compare
| # Adds headers to be sent to the proxy | ||
| def add_proxy_connect_headers | ||
| @headers.each do |field, value| | ||
| next unless ['Host', 'User-Agent'].include? field |
There was a problem hiding this comment.
It's better to move this array into a constant and make it a set:
PROXY_HEADERS_WHITELIST = %w(Host User-Agent).to_set.freeze
Connorhd
force-pushed
the
https-through-proxy-support
branch
5 times, most recently
from
f78df33
to
fa64bdb
Compare
|
@Connorhd I have rebased and cleaned up a bit your PR. By some reason 3 examples are failing with "IOError: problem making HTTP request: end of file reached" now. Please, see branch: /cc @zanker |
Connorhd
force-pushed
the
https-through-proxy-support
branch
from
fa64bdb
to
031fc01
Compare
|
@ixti I've fixed the tests on your rebase, one of the condition changes got lost in the rebase, plus the ssl tests need the ssl client to be configured now. |
|
👍 from me |
| @connection.send_request(req) | ||
| @connection.read_headers! | ||
|
|
||
| unless @connection.failed_proxy_connect? |
There was a problem hiding this comment.
A failed proxy connection should be an error, rather than soft failing I think. It's the equivalent of a socket error in that you're saying to connect to X and it failed.
If the concern is around accessing the response for further debugging, we can do something like what Net::HTTP does and include response as part of the error.
There was a problem hiding this comment.
It's unclear to me what the correct behavior is here. This case includes the 407 response when proxy authentication fails (which we have a test case for), its also consistent with the current behavior for http proxies.
i.e.
HTTP.get('http://not-real-sldjflkdjlkds.com') => exception
HTTP.via(proxy.addr, proxy.port).get('http://not-real-sldjflkdjlkds.com') => 500 response
There was a problem hiding this comment.
Okie-doke. If it's consistent with what we have then 👍
|
Besides that looks good to me. If you could fix those two things can merge. Thanks for working through this! |
Connorhd
force-pushed
the
https-through-proxy-support
branch
from
031fc01
to
4bfa1ee
Compare
Connorhd
force-pushed
the
https-through-proxy-support
branch
from
4bfa1ee
to
6aed65f
Compare
|
@zanker Updated with those changes |
First attempt at supporting https connections through proxy
|
Awesome! Thanks! |
|
Hi guys, trying to test this out in irb using version 0.8.5 and it hangs whenever I try and access an HTTPS endpoint. Has anybody got this working against a real proxy server? It works fine from curl on the command line. (Sample HTTP proxies taken from http://proxylist.hidemyass.com/search-1297445#listable) |
|
Thanks for report! I confirm bug exists. |
|
@timrwilliams Fixed in master. Will cut new release today or tomorrow. |
|
Nice, I've tested master and it works like a charm now. We've got an issue over on the Twitter gem (sferik/twitter-ruby#663) that needs this so cutting a new release would be really helpful. |
|
@timrwilliams OK. |
|
@timrwilliams Released as 0.8.8 |
This is pretty rough but I wanted to see if this looks like I'm at all going in the right direction. I added a failing test which now passes, so I think this is at least functional. Any feedback on what I need to do to get this into a mergable state would be great.
Connecting to https servers through a http proxy requires sending a CONNECT request to the proxy to setup a tunnel, then performing a normal https request through that tunnel. This PR adds the code to send that CONNECT request before continuing with the normal request.